-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ScrollView observes and adjusts for keyboard position #55
Conversation
BlueprintUICommonControls/Tests/Sources/Internal/KeyboardObserverTests.swift
Outdated
Show resolved
Hide resolved
BlueprintUICommonControls/Sources/Internal/KeyboardObserver.swift
Outdated
Show resolved
Hide resolved
BlueprintUICommonControls/Sources/Internal/KeyboardObserver.swift
Outdated
Show resolved
Hide resolved
BlueprintUICommonControls/Tests/Sources/Internal/KeyboardObserverTests.swift
Outdated
Show resolved
Hide resolved
BlueprintUICommonControls/Sources/Internal/KeyboardObserver.swift
Outdated
Show resolved
Hide resolved
Pushed some, but not all requested changes, will get to those later. This update also includes changes to how we manage |
ff932db
to
d42c5c1
Compare
BlueprintUICommonControls/Sources/Internal/KeyboardObserver.swift
Outdated
Show resolved
Hide resolved
Could you verify this isn't also an issue with the implementation here? |
any movement on this? |
Nope... can try to look soon. |
Doesn't seem to be an issue from testing EDIT: Spoke too soon.. wasn't seeing this on iphone, but it is happening on ipad. Will fix. |
This should be good for another review. |
BlueprintUICommonControls/Sources/Internal/KeyboardObserver.swift
Outdated
Show resolved
Hide resolved
175a19b
to
141eabf
Compare
* master: Releasing 0.7.0 Fix comment about extraSize ASCII diagrams in stack layout Measure stack axes separately A better workaround for missing dylibs Workaround for libswiftsimd.dylib not being copied Bump CocoaPods version Update to Xcode 11 and Swift 5.1 Remove iOS 10 availability checks Raise the minimum iOS version to 10 Releasing 0.6.0 Don't apply keyboardDismissMode unless it changed Change property from Bool to UIScrollView.KeyboardDismissMode and pass it directly Allow ScrollView to dismiss the keyboard on tap
self.center = center | ||
|
||
/// We need to listen to both `will` and `keyboardDidChangeFrame` notifications. Why? | ||
/// When dealing with an undocked or floating keyboard, moving the keyboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What actually happens with an undocked keyboard? We don't want to treat it as a bottom content inset in those cases, do we? e.g. if the keyboard is floating near the top of the screen, we should probably just leave the content offset alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and forth on this... there's four forms of keyboards:
- Docked, takes up full width
- Undocked, takes up full width
- Also undocked, split, also semantically is full width
- Floating on iPad, does not take up full width
I figured we generally want this for the first three cases, so kept it simple for now. I'll follow up with something for #4, to not inset in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
extension KeyboardObserver | ||
{ | ||
struct NotificationInfo : Equatable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be internal for testing access.
BlueprintUICommonControls/Tests/Sources/Internal/KeyboardObserverTests.swift
Outdated
Show resolved
Hide resolved
|
||
private extension UIView { | ||
|
||
var bp_safeAreaInsets : UIEdgeInsets { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smells like Obj-C. How about a name like polyfillSafeAreaInsets
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather keep this as is... It's clear to developers from patterns what this means.
// MARK: Keyboard | ||
// | ||
|
||
private func updateBottomContentInsetWithKeyboardFrame() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost the same func as applyContentInset
, can they merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh, will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to dig into this more... there's a bit of nuanced behaviour between the logic in applyContentInset
handling changes only from the model updates, vs updateBottomContentInsetWithKeyboardFrame
always updating on live views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to leave as is for now and come back to this one.
|
||
Note: When `keyboardAdjustmentMode` is used, it will also adjust | ||
the on-screen `UIScrollView`s `contentInset.bottom` to make space for the keyboard. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for comments!
BlueprintUICommonControls/Sources/Internal/KeyboardObserver.swift
Outdated
Show resolved
Hide resolved
BlueprintUICommonControls/Sources/Internal/KeyboardObserver.swift
Outdated
Show resolved
Hide resolved
7a5a39b
to
95633fd
Compare
This PR makes
ScrollView
smart about observing the position of the system keyboard. When the keyboard is displayed, updated, or dismissed, theScrollView
will adjust itscontentInset.bottom
to account for the updated inset. This is done throughKeyboardObserver
, which is an object to observe and manage keyboard positioning, which I've copied over from Listable. I've also addedkeyboardDismissMode
toScrollView
in this PR, allowing management of keyboard dismissal inScrollView
instances.